Buildings code rewrite #3426

meased · 2018-10-05T02:34:41Z

(Most likely) a prerequisite for #2532

This PR simplifies the buildings SQL query and moves all styling decisions out of SQL and into CSS (as the software gods intended 😉).

No visual changes.

SQL

Before, normal buildings and major building were queried separately with each query specifically omitting the results of the other. This made it difficult to redefine the categories (and very difficult to add a third category).

The SQL query now queries all buildings together, along with any fields needed to make styling decisions:

     (SELECT
           way,
           building,
           amenity,
           aeroway,
           aerialway,
           tags->'public_transport' as public_transport
         FROM planet_osm_polygon
         WHERE building IS NOT NULL
           AND building != 'no'
           AND way_area > 1*!pixel_width!::real*!pixel_height!::real
         ORDER BY COALESCE(layer,0), way_area DESC
       ) AS buildings

CSS

Now that the buildings class has access to all the fields needed, the CSS can be written in one block. As you can see, it is now very easy to reclassify buildings and add/remove the minor class:

  #buildings {
    [zoom >= 13] {
      polygon-fill: @building-low-zoom;
      polygon-clip: false;
      line-width: 0;
      [zoom >= 15] {
        polygon-fill: @building-fill;
        line-color: @building-line;
        line-width: .75;
        line-clip: false;
      }
      [amenity = 'place_of_worship'],
      [aeroway = 'terminal'],
      [aerialway = 'station'],
      [building = 'train_station'],
      [public_transport = 'station'] {
        polygon-fill: @building-major-fill;
        line-color: @building-major-line;
      }
    }
  }

jeisenbe · 2018-10-05T03:30:15Z

Great work on the code! I think it might be better with the major buildings a little more prominent. Could we try a color half-way between the old and new major building colors?

…

On Fri, Oct 5, 2018 at 11:34 AM meased ***@***.***> wrote: Fixes #2532 <#2532> This PR simplifies the buildings SQL query and moves all styling decisions out of SQL and into CSS (as the software gods intended 😉). Visual changes: - Major buildings are not as dark. - Adds a minor building category (lighter). SQL Before, normal buildings and major building were queried separately with each query specifically omitting the results of the other. This made it difficult to redefine the categories (and very difficult to add a third category). The SQL query now queries all buildings together, along with any fields needed to make styling decisions: (SELECT way, building, amenity, aeroway, aerialway, tags->'public_transport' as public_transport FROM planet_osm_polygon WHERE building IS NOT NULL AND building != 'no' AND way_area > 1*!pixel_width!::real*!pixel_height!::real ORDER BY COALESCE(layer,0), way_area DESC ) AS buildings CSS Now that the buildings class has access to all the fields needed, the CSS can be written in one block. As you can see, it is now very easy to reclassify buildings and add/remove the minor class: #buildings { [zoom >= 13] { polygon-fill: @building-low-zoom; polygon-clip: false; line-width: 0; [zoom >= 15] { polygon-fill: @building-fill; line-color: @building-line; line-width: .75; line-clip: false; } [amenity = 'place_of_worship'], [aeroway = 'terminal'], [aerialway = 'station'], [building = 'train_station'], [public_transport = 'station'] { polygon-fill: @building-major-fill; line-color: @building-major-line; } [building = 'garage'], [building = 'garages'], [building = 'carport'], [building = 'shed'], [building = 'greenhouse'], [building = 'farm_auxiliary'], [building = 'construction'], [building = 'service'], [building = 'ger'], [building = 'ruins'] { polygon-fill: @building-minor-fill; line-color: @building-minor-line; } } } Test Renderings https://www.openstreetmap.org/#map=18/45.59116/-122.74872 (Images line up so you can put them in browser tabs and switch tabs back and forth.) Current style on top. [image: sj_14b] <https://user-images.githubusercontent.com/14190496/46512926-21ad4880-c80b-11e8-9bd5-ce79a5a91950.png> [image: sj_14a] <https://user-images.githubusercontent.com/14190496/46512928-24a83900-c80b-11e8-9f8e-42587ae7b11d.png> [image: sj_15b] <https://user-images.githubusercontent.com/14190496/46512932-27a32980-c80b-11e8-8553-52a324fbff8a.png> [image: sj_15a] <https://user-images.githubusercontent.com/14190496/46512934-2a058380-c80b-11e8-8ed8-916e98be4b78.png> [image: sj_16b] <https://user-images.githubusercontent.com/14190496/46512936-2bcf4700-c80b-11e8-827a-5b0b0a33ada3.png> [image: sj_16a] <https://user-images.githubusercontent.com/14190496/46512937-2d990a80-c80b-11e8-9f73-165189c816d4.png> [image: sj_17b] <https://user-images.githubusercontent.com/14190496/46512939-2ffb6480-c80b-11e8-9501-9f829564db6a.png> [image: sj_17a] <https://user-images.githubusercontent.com/14190496/46512945-31c52800-c80b-11e8-81c8-7ef24abcc77d.png> [image: sj_18b] <https://user-images.githubusercontent.com/14190496/46512947-32f65500-c80b-11e8-927e-dd44db95486e.png> [image: sj_18a] <https://user-images.githubusercontent.com/14190496/46512950-34c01880-c80b-11e8-8054-73d2066c7941.png> https://www.openstreetmap.org/#map=16/45.5879/-122.5936 [image: pa_16b] <https://user-images.githubusercontent.com/14190496/46513013-7ea8fe80-c80b-11e8-9c50-b90f7d78de4c.png> [image: pa_16a] <https://user-images.githubusercontent.com/14190496/46513014-7fda2b80-c80b-11e8-9e0e-80d8893805b4.png> [image: pa_17b] <https://user-images.githubusercontent.com/14190496/46513017-810b5880-c80b-11e8-8d43-5f445e98550f.png> [image: pa_17a] <https://user-images.githubusercontent.com/14190496/46513020-82d51c00-c80b-11e8-9e2c-d4ff7d16dffc.png> ------------------------------ You can view, comment on, or merge this pull request online at: #3426 Commit Summary - Standard text halo for fitness_center and fitness_station - Merge branch 'master' of github.com:meased/openstreetmap-carto - Merge https://github.com/gravitystorm/openstreetmap-carto - xMerge https://github.com/gravitystorm/openstreetmap-carto - Merge https://github.com/gravitystorm/openstreetmap-carto - Rewrite buildings query, support for minor buildings - Adjust colors - Add comment - Original building color File Changes - *M* buildings.mss <https://github.com/gravitystorm/openstreetmap-carto/pull/3426/files#diff-0> (39) - *M* project.mml <https://github.com/gravitystorm/openstreetmap-carto/pull/3426/files#diff-1> (31) Patch Links: - https://github.com/gravitystorm/openstreetmap-carto/pull/3426.patch - https://github.com/gravitystorm/openstreetmap-carto/pull/3426.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3426>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AoxshM8MWXdk5DzEWYOHKTqYaib97KWCks5uhsVHgaJpZM4XJdL7> .

kocio-pl · 2018-10-05T04:13:53Z

I like the idea of these complex changes. Did you however test the performance? We use SQL a lot to achieve reasonable speed.

kocio-pl · 2018-10-05T04:33:42Z

Could you also test minor buildings darker, like half the way between the proposed color and standard buildings?

meased · 2018-10-05T21:02:06Z

Minor buildings no change:

Minor buildings 4% lighter:

Minor buildings 7% lighter (same as screen shots above):

meased · 2018-10-05T21:14:36Z

Major buildings no change:

Major buildings 15% darker:

Major buildings 10% darker (same as screenshots above):

As you can see, I was more concerned with fixing the dreadful "gastronomy in the airport" problem than I was with spotting churches in neighbourhoods.

meased · 2018-10-05T21:45:08Z

Did you however test the performance?

No.

Anyone know how to run benchmarks in docker?

jeisenbe · 2018-10-05T23:14:30Z

Would it make sense to lighten major buildings at higher zoom levels? At mid-zoom it is very helpful that the airport terminal building is obvious, and that’s when it is useful for places of worship etc. But at zoom 18 there is no reason for the major building to stay so dark. Would that be too complicated ?

…

On Sat, Oct 6, 2018 at 6:45 AM meased ***@***.***> wrote: Did you however test the performance? No. Anyone know how to run benchmarks in docker? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3426 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AoxshCBb_Z9SgjZ3WC1j5qpWh9E_4cxRks5uh9LngaJpZM4XJdL7> .

meased · 2018-10-05T23:33:58Z

That's not that complicated. Worth a test rendering at least.

kocio-pl · 2018-10-06T00:06:31Z

I think both darker versions (4% for minor and 15% for major) work better. Since buildings are very popular map feature, we need much more testing in different areas (backgrounds).

I like the idea of minor buildings instead of problems with ruins icon, especially for castles (see #331).

Unfortunately we have no performance framework (see #1287). However we can at least try to deploy it on some real server - @rrzefox, could you measure the effect of replacing some SQL code with Carto CSS here?

matthijsmelissen · 2018-10-06T08:05:21Z

See also #68 and #490.

I'm not sure if I really see the advantage of rendering minor buildings lighter.

mboeringa · 2018-10-06T08:44:09Z

I'm not sure if I really see the advantage of rendering minor buildings lighter.

That surprises me. To me it makes utter sense to render relatively insignificant non-living quarters like small garden sheds and garages lighter.

It might actually stimulate good tagging practices in that people actually start to distinguish them in tagging, and add building=shed instead of just generic building=yes.

Tomasz-W · 2018-10-06T11:13:01Z

@meased Can you test lighter outline for minor buildings? It looks a little bit like some holes in ground currently.

mboeringa · 2018-10-06T17:45:28Z

@meased Can you test lighter outline for minor buildings? It looks a little bit like some holes in ground currently.

I think that is mainly a consequence of the relatively dark grey of the landuse=residential, which appears just a bit darker than the minor buildings. So either slightly lightening up of landuse=residential, or slightly darkening the minor buildings fill, might help.

meased · 2018-10-06T18:03:28Z

I think that is mainly a consequence of the relatively dark grey of the landuse=residential

Exactly.

I choose landuse=residential and airports for examples because they represent the worst cases for how light you can go on one end and how dark you can go on the other. There is not much space to play with in the middle. The minor buildings above would have looked better on bare background:

kocio-pl · 2018-10-06T23:32:14Z

It might actually stimulate good tagging practices in that people actually start to distinguish them in tagging, and add building=shed instead of just generic building=yes.

I also believe it's more pleasant, less cluttering and more useful at the same time, so it's a triple win for me.

There are a lot of auxiliary buildings, but still most of them is tagged with building=yes (only about 50 mln out of 300 mln of all the buildings have any other value!). I don't see how could we attract people to more precise tagging other than showing them a bit different as a "minor" super-class.

@meased I see that you have added carpark type, but omitted collapsed - was there any reason for this? I have tried to stay above 10k limit, but we don't have to stick to it, there are also some other types that make sense for me, what do you think about it?

jragusa · 2018-10-07T12:59:19Z

Please also consider underground #552 and roof #2475 buildings to get a consistent rendering.

meased · 2018-10-07T17:02:16Z

I got the original minor buildings list from #2532. I was avoiding underground (because some people don't want it rendered at all) and roof (because #2475 is different than just rendering them lighter). But until those other issues are sorted out I don't see why they wouldn't qualify as minor buildings in the meantime.

@kocio-pl: Strangly, collapsed isn't on the wiki page. I included it anyway because the usage is very high.

The list of minor buildings is now:

    [building = 'roof'],
    [building = 'construction'],
    [building = 'damaged'],
    [building = 'collapsed'],
    [building = 'underground'],
    [building = 'service'],
    [building = 'slurry_tank'],
    [building = 'shed'],
    [building = 'garage'],
    [building = 'garages'],
    [building = 'farm_auxiliary'],
    [building = 'carport'],
    [building = 'barn'],
    [building = 'stable'],
    [building = 'cowshed'],
    [building = 'greenhouse'],
    [building = 'ger'],
    [building = 'ruins'] {

meased · 2018-10-07T17:42:37Z

Playing with @jeisenbe's idea of variable major building darkness:

14

15

16

17

Tomasz-W · 2018-10-07T17:54:06Z

About test renderings above: I find darkness of major buildings on high zoom levels definetely too low. When I looked at them with "fresh eye" I had to search for major buildings, but they should be distinguishable intuitively.
Just to be clear: I very like the idea proposed in this PR, it just need some tuning IMO.

HolgerJeromin · 2018-10-07T23:56:49Z

buildings.mss

+    [building = 'stable'],
+    [building = 'cowshed'],
+    [building = 'greenhouse'],
+    [building = 'ger'],


What is that?

I miss =hut and perhaps =cabin in the list.

You mean ger? See https://en.wikipedia.org/wiki/Yurt

meased · 2018-10-14T16:29:17Z

More testing (at 7% lighter):

https://www.openstreetmap.org/#map=17/45.58231/-122.73927

https://www.openstreetmap.org/#map=17/45.15308/-122.84857

https://www.openstreetmap.org/#map=18/45.46846/-122.73448

https://www.openstreetmap.org/#map=17/45.50819/-122.64908

meased · 2018-10-14T16:38:32Z

@Tomasz-W wanted to see 5% lighter:

0%

5%

7%

0%

5%

7%

Tomasz-W · 2018-10-14T16:55:45Z

Thanks! As 5% is too dark to distinguish them fastly, but 7% is minimally too light (too prominent) on residential areas, I would go with middle one 6%.

meased · 2018-10-14T17:19:28Z

6% version of the last image:

jeisenbe · 2018-10-14T23:57:31Z

Looks good on residential landuse. Can we try an example with minor buildings on Religious landuse?

…

On Mon, Oct 15, 2018 at 2:19 AM meased ***@***.***> wrote: 6% version of the last image: [image: la2_6p] <https://user-images.githubusercontent.com/14190496/46919842-3ff90e00-cf9a-11e8-9d71-87f017a21491.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3426 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AoxshNMFTgzx5RQ88T8qrGWGs94PD4yuks5uk3IjgaJpZM4XJdL7> .

Adamant36 · 2018-10-15T05:53:47Z

@meased, can you try a couple of tests with 6% and the new yellowish societal amenities color?

kocio-pl · 2018-10-15T05:59:33Z

I think it's important to test it on different backgrounds where one can find buildings (for example landuse=garages or landuse=commercial), not just the bare ground. They will look different there, so it's too early for precise shade tuning.

kocio-pl · 2018-10-16T18:00:45Z

@meased My proposition is to start with just the code restructuring as a separate PR, so testing would be easier and less error prone, and next making another PR based on this which will be focused only on making minor category and visual testing the colors.

meased · 2018-10-17T01:07:00Z

My proposition is to start with just the code restructuring as a separate PR

I think I'll do this.

meased · 2018-10-17T01:14:50Z

Done.

This PR is now just a rewrite of the buildings SQL and style sheet. There should be no visual differences. I have tested this in Kosmtik by flipping the overlay of the standard map on and off and I can't see any difference on any zoom level.

kocio-pl · 2018-10-17T01:17:54Z

Great! Since there is a change from SQL code to CSS code, there might be some performance difference. @rrzefox Could you test this difference?

kocio-pl · 2018-10-17T05:04:32Z

Well, this statement was not accurate - it's rather simpler both SQL and CSS code, so there should be no problems with performance. I will just wait with this until v4.16.0 is out (I plan to do this 19.10) and probably merge this PR soon after that. Then we can start testing minor buildings PR.

kocio-pl · 2018-10-19T02:20:30Z

Now we can restart working on new buildings categorization.

kocio-pl · 2018-10-20T06:16:45Z

BTW: making major buildings lighter would also fix #3071.

kocio-pl · 2019-01-03T05:59:56Z

@meased What about further tuning? Do you plan to get back to the proposed minor and major buildings changes?

meased · 2019-01-04T21:59:25Z

The problem here is that buildings need to simultaneously serve as a background for our lightest foreground (I believe that is gastromony #C77400) and as a foreground to our darkest landusage (which I believe is religous #d0d0d0, but residential is also a problem), all while having three brightnesses of minor, normal, and major which are all distinguishable from each other.

I'm not convinced that all of these criteria can be met, hence the inactivity.

Regardless, I could start a PR so that these problems could be investigated.

matthijsmelissen · 2019-01-04T22:17:03Z

That's the kind of thinking we need! Feel free to propose dropping/altering any of these criteria if you think it would make sense.

By the way, just curious, what's your username on openstreetmap.org?

meased · 2019-01-04T23:33:31Z

what's your username on openstreetmap.org?

cowdog

Tomasz-W · 2019-01-05T08:43:14Z

Please remember about discussed half-transparency for some buildings (#552 (comment)).

meased added 9 commits July 25, 2018 19:25

Standard text halo for fitness_center and fitness_station

3f51aa8

Merge branch 'master' of github.com:meased/openstreetmap-carto

5d690ef

Merge https://github.com/gravitystorm/openstreetmap-carto

1f52710

xMerge https://github.com/gravitystorm/openstreetmap-carto

34f05e4

Merge https://github.com/gravitystorm/openstreetmap-carto

eab9183

Rewrite buildings query, support for minor buildings

d66f9a3

Adjust colors

ea68394

Add comment

6f86fef

Original building color

e0cfe66

Add more types to minor buildings

37589b2

HolgerJeromin reviewed Oct 7, 2018

View reviewed changes

Return major building to their original color.

46a6f7b

meased changed the title ~~Buildings code rewrite, major/normal/minor style changes~~ Lighten minor buildings, buildings code rewrite Oct 14, 2018

Remove minor building styling.

19d758b

meased changed the title ~~Lighten minor buildings, buildings code rewrite~~ Buildings code rewrite Oct 17, 2018

kocio-pl merged commit 6ac2d9c into gravitystorm:master Oct 19, 2018

jragusa mentioned this pull request Nov 27, 2018

Render underground buildings different #552

Open

meased deleted the buildings branch January 4, 2019 21:59

meased mentioned this pull request Jan 21, 2019

"The map appears washed out, more contrast wanted" #3647

Open

meased mentioned this pull request Jan 28, 2019

Lighten major buildings fill and outline #3659

Merged

jeisenbe mentioned this pull request Feb 12, 2019

Differentiate rendering of minor buildings with lighter fill and lighter outline #3679

Closed

jc86035 mentioned this pull request Apr 11, 2019

Rendering of building=train_station and railway=platform/highway=platform(?) does not respect layers #2504

Closed

Buildings code rewrite #3426

Buildings code rewrite #3426

Conversation

meased commented Oct 5, 2018 • edited Loading

SQL

CSS

jeisenbe commented Oct 5, 2018 via email

kocio-pl commented Oct 5, 2018

kocio-pl commented Oct 5, 2018

meased commented Oct 5, 2018

meased commented Oct 5, 2018

meased commented Oct 5, 2018

jeisenbe commented Oct 5, 2018 via email

meased commented Oct 5, 2018

kocio-pl commented Oct 6, 2018

matthijsmelissen commented Oct 6, 2018

mboeringa commented Oct 6, 2018

Tomasz-W commented Oct 6, 2018 • edited Loading

mboeringa commented Oct 6, 2018

meased commented Oct 6, 2018

kocio-pl commented Oct 6, 2018 • edited Loading

jragusa commented Oct 7, 2018

meased commented Oct 7, 2018

meased commented Oct 7, 2018 • edited Loading

Tomasz-W commented Oct 7, 2018 • edited Loading

HolgerJeromin Oct 7, 2018

Choose a reason for hiding this comment

kocio-pl Oct 8, 2018

Choose a reason for hiding this comment

meased commented Oct 14, 2018

meased commented Oct 14, 2018

Tomasz-W commented Oct 14, 2018 • edited Loading

meased commented Oct 14, 2018

jeisenbe commented Oct 14, 2018 via email

Adamant36 commented Oct 15, 2018 • edited Loading

kocio-pl commented Oct 15, 2018

kocio-pl commented Oct 16, 2018

meased commented Oct 17, 2018

meased commented Oct 17, 2018

kocio-pl commented Oct 17, 2018

kocio-pl commented Oct 17, 2018

kocio-pl commented Oct 19, 2018

kocio-pl commented Oct 20, 2018

kocio-pl commented Jan 3, 2019

meased commented Jan 4, 2019

matthijsmelissen commented Jan 4, 2019

meased commented Jan 4, 2019

Tomasz-W commented Jan 5, 2019

meased commented Oct 5, 2018 •

edited

Loading

Tomasz-W commented Oct 6, 2018 •

edited

Loading

kocio-pl commented Oct 6, 2018 •

edited

Loading

meased commented Oct 7, 2018 •

edited

Loading

Tomasz-W commented Oct 7, 2018 •

edited

Loading

Tomasz-W commented Oct 14, 2018 •

edited

Loading

Adamant36 commented Oct 15, 2018 •

edited

Loading